Skip to content

fix(memory): stop rewriting the FTS5 trigger on every open (#366) - #979

Open
ziomik wants to merge 1 commit into
bradbrok:mainfrom
ziomik:fix/366-fts5-init-race
Open

fix(memory): stop rewriting the FTS5 trigger on every open (#366)#979
ziomik wants to merge 1 commit into
bradbrok:mainfrom
ziomik:fix/366-fts5-init-race

Conversation

@ziomik

@ziomik ziomik commented Aug 2, 2026

Copy link
Copy Markdown

Fixes #366.

Il problema

_init_schema eseguiva DROP TRIGGER + CREATE TRIGGER su reflections_au a ogni apertura di ReflectionStore. Diversi componenti ne costruiscono uno all'avvio del daemon, quindi due potevano sovrapporsi in quella finestra: il perdente riceveva trigger reflections_au already exists e l'except sqlite3.OperationalError troppo largo lo interpretava come "FTS5 non compilato in questa build".

Risultato: quel processo restava con _fts5_available = False per tutta la sua vita, con recall() degradato a LIKE (niente ranking BM25) e nessun segnale oltre a una riga di warning che diceva la cosa sbagliata.

Il fix

1. Non riscrivere il trigger quando e' gia' aggiornato (_migrate_fts_update_trigger)

Il trigger reflections_au e' stato estratto da _FTS5_TRIGGERS in una costante propria: a differenza degli altri due non puo' usare CREATE ... IF NOT EXISTS, perche' i DB piu' vecchi hanno una versione che scattava a ogni UPDATE (l'access tracking faceva churn dell'indice FTS) e va davvero sostituita.

Ora si legge prima sqlite_master: se la definizione e' gia' quella corrente non si scrive nulla, quindi su un DB aggiornato la finestra di race sparisce del tutto. Un DB che ha davvero bisogno di migrare prende un BEGIN IMMEDIATE e ricontrolla sotto il lock, cosi' gli opener concorrenti si serializzano ed esattamente uno esegue la riscrittura.

execute() statement per statement invece di executescript(), che farebbe un COMMIT implicito annullando la transazione.

2. Restringere l'except

Solo no such module significa FTS5 assente: quel caso continua a degradare a LIKE come da #295. Ogni altro OperationalError (errore di I/O, corruzione, timeout sul lock) ora propaga invece di essere scambiato per "FTS5 non disponibile".

Test

Nuovo file tests/test_memory_fts_init_race.py (5 test):

  • la riapertura di un DB aggiornato non esegue DDL sul trigger
  • 8 opener concorrenti mantengono tutti FTS5 ed esattamente uno migra
  • la ricerca keyword continua a essere ordinata da BM25 dopo la riapertura (la meta' visibile all'utente: non e' caduta su LIKE)
  • un disk I/O error non viene scambiato per FTS5 mancante
  • una build senza FTS5 degrada ancora in modo grazioso (non-regressione su Replace bare excepts in pinky_memory/store.py with logged specific-exception handlers #295)

Il test concorrente asserisce "esattamente una migrazione" tracciando l'SQL: e' deterministico proprio grazie al ricontrollo sotto lock, e senza quell'asserzione passerebbe anche con il codice pre-fix.

Verifica eseguita:

  • pre-fix (solo i test, produzione stashata): 3 failed, 2 passed
  • post-fix: 5 passed
  • non-regressione: test_memory_store.py + test_memory_server.py -> 174 passed; le altre 8 suite che toccano pinky_memory (test_kg_*, test_memory_cross_agent, test_memory_heal_unembedded, test_shared_mcp) -> 290 passed
  • ruff check pulito su entrambi i file

Note

Branch basato su origin/main, non sulla linea di produzione (bloccata da GH013 per una chiave Supabase in history).

Fuori scope, tracciato a parte in #368: anche _migrate_backfill_review_schedule esegue una UPDATE incondizionata a ogni apertura dello store, non solo alla prima.


🤖 Opened by Engineer

)

_init_schema ran DROP TRIGGER + CREATE TRIGGER for reflections_au on every
ReflectionStore open. Several components construct a store at daemon start,
so two could overlap in that window: the loser hit "trigger reflections_au
already exists", and the over-broad `except sqlite3.OperationalError` read
that as "FTS5 is not compiled in" — leaving that process with keyword search
silently degraded to LIKE for its whole lifetime.

- Read sqlite_master first and skip the rewrite when the trigger is already
  current, which removes the window entirely for an up-to-date DB. A DB that
  genuinely needs migrating takes a BEGIN IMMEDIATE and re-checks under the
  lock, so racing openers serialize and exactly one performs the rewrite.
  (execute() statement by statement: executescript() would COMMIT implicitly.)
- Narrow the except to "no such module" so only a missing FTS5 build degrades
  to LIKE; any other OperationalError now surfaces instead of being swallowed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant